-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Query Descriptor Visibility #911
Merged
ryanrath
merged 9 commits into
ubccr:xdmod8.5
from
ryanrath:fix_query_descriptor_visibility
May 14, 2019
Merged
Fix Query Descriptor Visibility #911
ryanrath
merged 9 commits into
ubccr:xdmod8.5
from
ryanrath:fix_query_descriptor_visibility
May 14, 2019
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryanrath
force-pushed
the
fix_query_descriptor_visibility
branch
2 times, most recently
from
May 3, 2019 16:47
2270daa
to
0ac5259
Compare
** acl-config ** Previously we only allowed statistics to determine if an `acl_group_by` record was visible or not. But our config files seem to indicate that certain query descriptors should be able to control their ability to be shown as well. To support both of these use cases the `processQueryDescriptors` function in `acl-config` has been updated to take into account the `hide` property defined at the query descriptor level and the `visible` property defined at the statistic level. The logic used to determine the final visibility of an `acl_group_by` record is as follows. - By default all entries are shown - If `visible` is defined then we use it's value. - Else, if `hide` is defined then we use it's value. Also, there was a minor refactor with the removal of the `$enabled` variable. The `$disable` variable that it was based on was never going to be null, so it's use could be replaced w/ just `!$disable`. ** Acls ** There was some minor cleanup done because it was determined that we always want to have the `visible` column included, not just when a `$statisticName` is provided. This allowed us to move the `$descripter->setShowMenu(...)` from out of the `if(isset($statisticName)) { ... }` guard so that all query descriptors generated by this function will have their `showMenu` property populated. These two sets of changes work together to allow us to control our UI state as we expect to.
Fixing issues introduced by ubccr#897. Mostly it was just updating hard coded paths, but there are a few places where the format of the expected data json files are important ( ETLv2 Directory Scanner Tests ). This required re-formatting of the affected files per their use case. The
The User Profile pops up to prompt the user to double check their email address. This means that the ext-mask-msg will be present until the user clicks close, thus causing a problem for the `Logout` step.
This is just the initial bootstrap commit for the new `get_menus` endpoint integration tests.
Added tests for `controllers/user_interface.php/operation=get_menus` and for `Acls::getDisabledMenus`. `get_menus` is the endpoint that provides the list of of query descriptors visible to the user. While `getDisabledMenus` provides the list of query descriptors that are disabled. This should provide us a with a set of happy path tests for these two sets of functionality that can later be expanded upon as needed.
aka. added newlines
ryanrath
force-pushed
the
fix_query_descriptor_visibility
branch
from
May 7, 2019 15:55
8a89bd5
to
b540d23
Compare
6 tasks
plessbd
reviewed
May 11, 2019
tests/artifacts/xdmod/acls/output/get_disabled_menus-centerdirector.json
Show resolved
Hide resolved
plessbd
reviewed
May 11, 2019
tests/artifacts/xdmod/etlv2/dataendpoint/input/directory_scanner/euca_2017-06-01.json
Show resolved
Hide resolved
plessbd
approved these changes
May 14, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: I apologize in advance for the large number of files in this PR. On the plus side, the vast majority of them are test artifact updates.
Description
Updated to allow query descriptors to control visibility
acl-config
Previously we only allowed statistics to determine if an
acl_group_by
recordwas visible or not. But our config files seem to indicate that certain query
descriptors should be able to control their ability to be shown as well. To
support both of these use cases the
processQueryDescriptors
function inacl-config
has been updated to take into account thehide
property definedat the query descriptor level and the
visible
property defined at thestatistic level. The logic used to determine the final visibility of an
acl_group_by
record is as follows.visible
is defined then we use it's value.hide
is defined then we use it's value.Also, there was a minor refactor with the removal of the
$enabled
variable.The
$disable
variable that it was based on was never going to be null, so it'suse could be replaced w/ just
!$disable
.Acls
There was some minor cleanup done because it was determined that we always want
to have the
visible
column included, not just when a$statisticName
isprovided.
This allowed us to move the
$descripter->setShowMenu(...)
from out of theif(isset($statisticName)) { ... }
guard so that all query descriptorsgenerated by this function will have their
showMenu
property populated.These two sets of changes work together to allow us to control our UI state as
we expect to.
Motivation and Context
Being able to control the visibility of the group bys / statistics via ACLs is expected. This was specifically a problem on an XSEDE install due to the more complicated visibility requirements. In OpenXDMoD there are no group by's / statistics that are hidden via the ACLs so this was not caught there.
Tests performed
Manual Testing was performed:
Usage
Tab.etc/roles.d/pub.json#query_descripters
Types of changes
Checklist: